-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client/{core,cmd/dexc}: load unfunded orders for recovery #967
Conversation
Note that there are two better changes that can be made eventually/alternatively:
See also 172dbb7#r46977835 |
This comment has been minimized.
This comment has been minimized.
bce82ff
to
5972945
Compare
Major change to this PR. The dexc/Core option is removed, and unfunded orders are now loaded into the trades map so that unaffected matches can be processed as usual, and the order and affected matches can be revoked via status resolution or server requests. |
coins, err := wallets.fromWallet.FundingCoins(byteIDs) | ||
if err != nil { | ||
notifyErr(SubjectOrderCoinError, "Source coins retrieval error for %s %s: %v", unbip(wallets.fromAsset.ID), tracker.token(), err) | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removal of these continue
s are the crux of this change, allowing this trackedTrade
to enter the trades map. The order is canceled automatically if it is a booked standing limit order to prevent new matches. Any matches that do not require funding coins will continue swap negotiation as usual, and matches needing funding coins are blocked with swapErr.
@@ -3695,7 +3763,7 @@ func generateDEXMaps(host string, cfg *msgjson.ConfigResult) (map[uint32]*dex.As | |||
} | |||
|
|||
// runMatches runs the sorted matches returned from parseMatches. | |||
func (c *Core) runMatches(dc *dexConnection, tradeMatches map[order.OrderID]*serverMatches) (assetMap, error) { | |||
func (c *Core) runMatches(tradeMatches map[order.OrderID]*serverMatches) (assetMap, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dexConnection
input became unused when refreshUser
was removed.
8b3caa6
to
3079fb7
Compare
1c9f0e0
to
ef208a6
Compare
This addresses the issue of orders that lose their funding coins one way or another (e.g. spend outside of dexc) can never be loaded. Prior to this PR, that meant two things: - On login, the user will be forever spammed by an "Order coin error" / "Source coins retrieval error". - Matches for such orders can never be recovered, even if they do not require funding coins. This made recovery a manual task if there were matches in progress or requiring refund, etc. Unfunded orders that require funding (epoch/booked or with matches needing to send swap txns) are explicitly blocked from negotiating new matches, and any offending matches that require funding coins are blocked with swapErr. However, notably, the trackedTrade is added to the dc.trades map, unlike before this change. In addition to allowing unaffected matches to proceed, match status resolution can be performed for the order and its matches, potentially revoking it, and revoke_order/revoke_matches requests from the server will be recognized. This allows recovery or completion of other unaffected matches belonging to the trade. The order and the unfunded matches stay active (but halted via swapErr) until they are either revoked or the user possibly resolves a wallet issue that made the funding coins inaccessible and restarts dexc to try loading the funding coins again. If the coins are not spent, they probably are using the wrong wallet that does not control the referenced coins. Unfunded standing limit orders that are either epoch or booked are also canceled to prevent further matches that will be likely to fail.
ef208a6
to
bb575e4
Compare
Thanks, @JoeGruffins, I had forgotten about this PR since the backport was already merged. I'm still catching up on review. |
if status := tracker.metaData.Status; status != order.OrderStatusEpoch && status != order.OrderStatusBooked { | ||
return fmt.Errorf("order %v not cancellable in status %v", oid, status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We removed this from the release-0.1
backport (already merged), but it's still on this master PR. What do you say to leaving @buck54321?
This is part 2 of a
resumeTrades
issue resolution following #965This addresses the issue of orders that lose their funding coins one way or another (e.g. spend outside of dexc) can never be loaded. Prior to this PR, that meant two things:
"Source coins retrieval error".
require funding coins. This made recovery a manual task if there were
matches in progress or requiring refund, etc.
Unfunded orders that require funding (epoch/booked or with matches needing to send swap txns) are explicitly blocked from negotiating new matches, and any offending matches that require funding coins are blocked with
swapErr
. However, notably, thetrackedTrade
is added to thedc.trades
map, unlike before this change. In addition to allowing unaffected matches to proceed, match status resolution can be performed for the order and its matches, potentially revoking it, and revoke_order/revoke_matches requests from the server will be recognized.This allows recovery or completion of other unaffected matches belonging to the trade. The order and the unfunded matches stay active (but halted via
swapErr
) until they are either revoked or the user possibly resolves a wallet issue that made the funding coins inaccessible and restarts dexc to try loading the funding coins again. If the coins are not spent, they probably are using the wrong wallet that does not control the referenced coins.Unfunded standing limit orders that are either epoch or booked are also canceled to prevent further matches that will be likely to fail.